Fix #15000: Propagate integral cast ranges with path-safe value flow - #8819

AI review

The PR introduces a new false negative that is easy to hit: arrayIndexOutOfBounds is no longer reported for a[i + 1] or a[2 * i] when i is an unsigned/size_t loop counter with a constant bound, for example for (unsigned i = 0; i < 10; i++) a[i + 1] = 0;. The merge base reports index 10. The PR reports nothing.

The loop range is set on i as a possible bound <=9 before the excluded range !>=10 is added. At that point isUnsignedArithmeticNoWrap cannot prove that the operation does not wrap, so skipUnsignedRangeBounds drops the <=9 bound at the +/*. An earlier review reported that the no-wrap proof misses easy cases, but only with knownConditionTrueFalse examples. This buffer-overrun regression on the plain unsigned loop pattern is not in comments.md, and no test covers it.

Apart from this, I found no new problems. The type changes in symboldatabase.cpp (assignment and shift result types, getCommonIntegralType) gave the same results as base on my samples, or fixed base false positives such as unsigned short u; u = u + 1; if (u == 0).

Testing

I built cppcheck and testrunner in pr/, and cppcheck in base/.

Test suites: TestCondition, TestValueFlow, TestBufferOverrun, TestStl, TestType, TestOther, TestFunctions, TestUninitVar and TestSymbolDatabase all pass with 0 failures. In test/cli/other_test.py, 208 tests pass, including the two that the PR adds. 10 tests fail because of this environment: the --rule tests (built without PCRE) and the test_redundant_file_reads tests (strace is missing).

New FN: arrayIndexOutOfBounds with unsigned loop index arithmetic

t3.c:

#include <stddef.h>
int a[10];
char buf[16];
void h1(void) { for (unsigned i = 0; i < 10; i++) a[i + 1] = 0; }
void h2(void) { for (int i = 0; i < 10; i++) a[i + 1] = 0; }
void h3(void) { for (size_t i = 0; i < 10; i++) a[i + 1] = 0; }
void h8(void) { for (unsigned i = 0; i < 10; i++) a[2 * i] = 0; }
void k1(void) { for (int i = 0; i < 10; i++) a[i + 1u] = 0; }
void k2(void) { for (size_t i = 0; i < sizeof(buf); i++) buf[i + 1] = 0; }

Command: cppcheck -q --enable=style,warning,portability --platform=unix64 --template='{line}:{id}:{message}' t3.c

Base reports all six functions (line numbers here refer to the snippet above):

4:arrayIndexOutOfBounds:Array 'a[10]' accessed at index 10, which is out of bounds.
5:arrayIndexOutOfBounds:Array 'a[10]' accessed at index 10, which is out of bounds.
6:arrayIndexOutOfBounds:Array 'a[10]' accessed at index 10, which is out of bounds.
7:arrayIndexOutOfBounds:Array 'a[10]' accessed at index 18, which is out of bounds.
8:arrayIndexOutOfBounds:Array 'a[10]' accessed at index 10, which is out of bounds.
9:arrayIndexOutOfBounds:Array 'buf[16]' accessed at index 16, which is out of bounds.

The PR reports only line 5, the signed int i loop with i + 1.

In --debug for h1, i has the values {!<=-1,0,<=9,!>=10} in both versions. On +, base has {!<=0,1,<=10,!>=11} and the PR has only {!<=-1,1}. The possible <=9 bound is dropped at the unsigned +.

These cases still give the warning with the PR: if (i < 10) a[i + 1], if (i == 9) {} a[i + 1], and unsigned j = i + 1; a[j] inside the loop.

Other checks (same as base, or better)

• Assignment and compound assignment types ((c += 1) == 256, (x = u) > 300), c++ on signed char, and shifts (1 << 40, ternary shift counts) give the same results as base.
• unsigned short u; u = u + 1; if (u == 0): base wrongly reports this as always false. The PR does not, which is correct.
• (int)x > 2147483647 with long long x: the warning ID changes from compareValueOutOfTypeRangeError to knownConditionTrueFalse. Both are correct.

Automated review of 7a618a4 by Claude. The pull request was built and tested locally.

AI review

I found two new problems; neither is in comments.md.

1. New false negative after for loops whose condition has &&/||. The new gate at lib/valueflow.cpp:5708 skips valueFlowForLoopSimplifyAfter whenever partialCond is true. The off-by-one in a linear search, for (i = 0; i < 10 && x[i]; i++) {} a[i] = 0;, is no longer reported. Base reports it as arrayIndexOutOfBounds at index 10. Base's value is too certain: it says "After for loop, i has value 10", and that is only one possible exit. The fix would be to keep the after-loop value as a possible value, not to drop it.
2. ValueType::getCommonIntegralType returns the operand's whole ValueType when no promotion is needed. This happens for unary ~/- and <</>>. Bitfield width, constness and reference come along with it. For struct S { unsigned b : 3; }, the ~s.b token now has valueType-bits="3" in the dump; base has 0. Dump consumers such as the MISRA addon read that value.

The #15000 reproducer is reported (as knownConditionTrueFalse). Apart from these two, the cast, arithmetic and plain-char cases I tried gave the same results as base, or better ones.

Testing

I built cppcheck and testrunner in pr/, and cppcheck in base/. Both built without errors.

Test suites: TestCondition, TestValueFlow, TestBufferOverrun, TestStl, TestType, TestOther, TestFunctions, TestUninitVar and TestSymbolDatabase all pass with 0 failures. In test/cli/other_test.py, I ran the tests the PR adds or touches (-k "missing_include or 15000 or unique_error or cast" and -k "floating or bounded or 6366"): 8 passed.

1. FN after a for loop with a compound condition (new in the PR)

lp.c:

int a[10];
void p4(int *x) { int i; for (i = 0; i < 10 && x[i]; i++) {} a[i] = 0; }
void p7(void)   { int i; for (i = 0; i < 10; i++) {} a[i] = 0; }

Command: cppcheck -q --enable=style,warning --platform=unix64 --template='{line}:{id}:{message}' lp.c

Base:

2:arrayIndexOutOfBounds:Array 'a[10]' accessed at index 10, which is out of bounds.   (note: After for loop, i has value 10)
3:arrayIndexOutOfBounds:Array 'a[10]' accessed at index 10, which is out of bounds.

PR: only the line 3 warning. The line 2 warning is gone.

2. Bitfield width leaks into the result type of ~

bf2.c:

void use(int);
struct S { unsigned int b : 3; };
void f5(struct S s) { if ((~s.b) < 0) use(5); }

Command: cppcheck --dump -q bf2.c; grep -o 'str="~"[^>]*' bf2.c.dump

• Base: valueType-type="int" valueType-sign="unsigned" valueType-bits="0"
• PR: valueType-type="int" valueType-sign="unsigned" valueType-bits="3"

I found no false-positive change caused by this in cppcheck's own checks. The problem shows in the dump that addons read.

Same as base, or better

• #15000 pattern: reported.
• (int64_t)(uint16_t)x + 1 > 65536 and -(int)(int8_t)x > 128: correctly reported as always false.
• (uint8_t)x + 1 == 256 and unsigned r = (uint8_t)a; r -= 1; r > 255: no false positives.
• x = cond ? 2 : 3 and possible-negative initializers used as for-loop starts: same as base.
• Compound assignment and ++ on narrow types: same as base. Base's existing false positive on uint16_t y; y++; if (y == 0) is still there, so the PR did not cause it.
• Error paths of several existing knownConditionTrueFalse warnings now include an extra "Assuming that condition … is not redundant" note, so their hashes change. That is why some testcondition.cpp assertions were relaxed to find(). This matches existing wording elsewhere, so I am not reporting it as a bug.

Automated review of 2be3096 by Claude. The pull request was built and tested locally.

AI review

One new problem: the classic signed-char bug c == 0xFF is no longer reported on any platform. Base gives compareValueOutOfTypeRangeError for char c; if (c == 0xFF), c == 200, c > 127 and *s == 0xEF on unix64, win64 and native. The PR reports none of these.

The rewritten check treats plain char as having unknown signedness even when the platform defines it (defaultSign == 's' on every built-in platform except unspecified). It then uses the union of the signed and unsigned ranges. An earlier review suggested that union, but only for the case where the sign is really unknown. The PR also changes the expected result of the existing regression test for #11592 (char c; if (c == 128) on unix64) from the warning to "". That drops a result someone deliberately added, and it is inconsistent with the PR's own value-flow code, which does use platform.defaultSign for plain char conversions.

Apart from this, I found no new problems. Everything else I tested matched base or improved on it, and the problems I saw are already in comments.md.

Testing

I built cppcheck and testrunner in pr/, and cppcheck in base/.

Test suites: TestCondition, TestValueFlow, TestBufferOverrun, TestType, TestStl, TestOther, TestFunctions, TestUninitVar and TestSymbolDatabase all pass with 0 failures.

Regression: plain char compared with a value that only fits an unsigned char

ch.c:

void use(int);
void e1(char c) { if (c == 0xFF) use(1); }
void e2(char c) { if (c == 200) use(2); }
void e3(const char *s) { if (*s == 0xEF) use(3); }
void e4(char c) { if (c > 127) use(4); }

Command: cppcheck -q --enable=style,warning --platform=unix64 --template='{line}:{id}:{message}' ch.c

Base (the same with --platform=win64 and --platform=native):

2:compareValueOutOfTypeRangeError:Comparing expression of type 'char' against value 255. Condition is always false.
3:compareValueOutOfTypeRangeError:Comparing expression of type 'char' against value 200. Condition is always false.
4:compareValueOutOfTypeRangeError:Comparing expression of type 'const char' against value 239. Condition is always false.
5:compareValueOutOfTypeRangeError:Comparing expression of type 'char' against value 127. Condition is always false.

PR: no output on unix64, win64 or native.

Other checks (same as base, or better)

• Out-of-range comparisons: I compared about 25 cases of compareValueOutOfTypeRange across signed, unsigned, bool and 64-bit types. Base reported some of them wrongly, and the PR fixes those:
  • signed char c == 0xFFFFFFFFu is no longer called always false.
  • i <= 0xFFFFFFFFu is now reported as always true.
  • int64_t x > 0x7FFF...LL is now reported as always false.
  • u > -1 now gets a correct message.
• Casts and arithmetic: I tried about 25 cast and arithmetic cases and found no new false positives. The PR also removes base's wrong t = x + 1u; t == 0 "always false".
• Float bounds: I tried about 18 cases (>=/<= conditions, sqrt/log/acos/1.0/x, float→int casts after a clamp). The only new result is if (x >= 2.0) {} acos(x) → invalidFunctionArg, which is correct.
• for loops: I tried &&/|| loop conditions and negative init values. The results match base, except for the unsigned-index case that comments.md already reports.
• Crash check: cppcheck -j8 on samples/ and test/cfg/std.c gave no internal errors or crashes.

Automated review of 76f6fb3 by Claude. The pull request was built and tested locally.

AI review

I found no new problems that change any warning. Many of my tests reproduced issues that are already in comments.md, or cases that the PR fixes. That includes base's wrong "always false" results for (uint64_t)x + 1 == 0, unsigned y = x - 6; y == 4294967295u and x - y > 10.

I found one small new detail. The bitfield-width leak that was reported for getCommonIntegralType still happens on the ++/-- path, which copies the operand's whole ValueType. For s.b++ on an unsigned b : 3 bitfield, the dump now has valueType-bits="3"; base has 0. I found no checker result that this changes. It only shows in dump files that addons read.

Testing

Build: cppcheck and testrunner in pr/, and cppcheck in base/. Both built without errors.

Unit tests: TestCondition, TestValueFlow, TestBufferOverrun, TestType, TestStl, TestOther, TestFunctions, TestUninitVar and TestSymbolDatabase all give Tests failed: 0. In test/cli/other_test.py, -k "15000 or missing_include or cast or floating or bounded or 6366" gives 7 passed.

Base vs PR comparisons

All runs use cppcheck -q --enable=style,warning --platform=unix64 unless noted.

• #15000 reproducer and variations: these are now reported as always false, and they are correct:
  • (int64_t)(uint32_t)x - 1LL > 4294967295LL
  • (int)(int16_t)x - 1 < -32769
  • (int)(uint16_t)x + 1 > 65536
  • (uint16_t)x + 1 > 65536
  • (long long)(unsigned long)x > 4294967295LL on win64 and unix32. On unix64 it is correctly not reported.
• Possible false positives I tried: about 25 C and C++ cases with casts and arithmetic, including static_cast, functional casts, enum class, wchar_t/char16_t, uintptr_t, uint64_t sign tricks, /, %, >> and &. I also tried 12 unsigned inc/dec and guard cases. None gave a new false positive.
  • The PR removes some wrong base results: (uint64_t)x + 1 == 0, unsigned y = x - 6; y == 4294967295u with x <= 5, and x - y > 10 with unsigned x, y <= 10.
  • (int32_t)x - 1 == 2147483647 (with uint32_t x) is newly reported as always false. That is only wrong if signed overflow is allowed, so I am not reporting it.
• Other areas, results identical to base:
  • for loops with negative or conditional init values. The only difference is that one message now says "Either the condition 'x<-5' is redundant…", which is more precise.
  • Float >=/<= conditions followed by other comparisons and casts.
  • Path combinations of conditional values.
  • Shift counts, including ternary shift counts.
  • Assignment, compound-assignment and inc/dec result types in conditions.
• Larger comparison: I ran base and PR with -j4 --library=posix,gnu --platform=unix64 on test/cfg/{std.c,std.cpp,posix.c,gnu.c,windows.cpp,qt.cpp,wxwidgets.cpp}, lib/token.cpp, lib/mathlib.cpp, tinyxml2.cpp and simplecpp.cpp. The output is identical (50 lines each), with no crashes.

Dump detail (new, minor)

bf.c:

struct S { unsigned int b : 3; };
int f(struct S s) { return (s.b = 5) + (s.b++); }

Command: cppcheck -q --dump bf.c and look at the ++ token.

• Base: str="++" valueType-type="int" valueType-sign="unsigned" valueType-bits="0"
• PR: str="++" valueType-type="int" valueType-sign="unsigned" valueType-bits="3"

= has bits="3" in both versions.

Automated review of a53609a by Claude. The pull request was built and tested locally.

AI review

New false positives: the new impossible bounds on integral casts go through unary ~ with the bound kind not flipped. ~ is a decreasing operation. The ~ handler in vf_settokenvalue.cpp applies ~ to the value but keeps bound. So the cast bound !>=256 turns into !>=-257, when it should be !<=-257. The result is wrong "always false" or "always true" conditions in common bit-mask code:

• if (~(uint32_t)x == 0xFFFFFFFFu): true for x == 0.
• uint32_t m = ~(uint32_t)x; if (m == 0xFFFFFFFFu)
• ~(uint8_t)x == -256
• ~(int)x == 2147483647

Base gives none of these warnings. The ~ bug already exists in base, but there it only shows with bounds from conditions (if (x < 0 || x > 255) return; if (~x == -256) is wrong in base too). With the PR, every integral cast reaches it. The fix is to swap Bound::Upper and Bound::Lower for non-point values in the ~ handler, or to skip non-point values there. A test with ~ applied to a cast would help.

Apart from this, I found no new problems. The other issues I saw are already in comments.md.

Testing

Build: I built cppcheck and testrunner in pr/, and cppcheck in base/. Both built without errors.

Tests:

• These suites all give Tests failed: 0: TestCondition, TestValueFlow, TestBufferOverrun, TestStl, TestType, TestOther, TestFunctions, TestUninitVar and TestSymbolDatabase.
• In test/cli/other_test.py, -k "15000 or missing_include or cast or floating or bounded or 6366" gives 7 passed.

False positives with ~ on a cast (new in the PR)

t5.c:

#include <stdint.h>
void use(int);
void m1(int x) { uint32_t m = ~(uint32_t)x; if (m == 0xFFFFFFFFu) use(1); }
void m2(int x) { if (~(uint32_t)x == 0xFFFFFFFFu) use(2); }

Command: cppcheck -q --enable=style,warning --platform=unix64 --template='{line}:{id}:{message}' t5.c

Base: no output. PR:

3:knownConditionTrueFalse:Condition 'm==0xFFFFFFFFu' is always false
4:knownConditionTrueFalse:Condition '~(uint32_t)x==0xFFFFFFFFu' is always false

Both conditions are true for x == 0.

t3.c / t6.cpp (same command):

void c4(int x) { if (~(uint8_t)x == -256) use(22); }        // true for x==255
void c8(int x) { if (~(uint8_t)x < -255) use(26); }         // false for x==0
void c7(int x) { if (~(int8_t)x == 127) use(25); }          // true for x==-128
void n4(int x) { if (~static_cast<uint32_t>(x) == 0xFFFFFFFFu) use(4); }
void n6(int x) { if ((int)~(uint8_t)x == -1) use(6); }      // true for x==0
void n7(long long x) { if (~(int)x == 2147483647) use(7); } // true for (int)x==INT_MIN

• The PR reports each of these as always false. It reports c8 as always true.
• Base reports none of them.

--debug for c4:

~ always !>=-257
( always {!<=-1,!>=256}

The cast's !>=256 is mapped to !>=-257, which says ~(uint8_t)x < -257 always. The correct result is !<=-257.

Same bug in base without casts: void d1(int x) { if (x < 0 || x > 255) return; if (~x == -256) use(1); } is reported as always false by both base and PR. So the PR does not create the bug, but it makes it much easier to reach.

Other checks:

• Cast and arithmetic cases gave no other new false positives. I tried %, /, &, |, <<, 100 - (uint8_t)x, -(uint8_t)x and -(int8_t)x. One of them, (int)(uint8_t)x - 300 > 0, is now correctly reported as always false.

Automated review of eee2e8c by Claude. The pull request was built and tested locally.

AI review

I found one new false positive: a value from an integral cast, decremented in place (y -= 1, y--), is reported as never wrapping. The PR adds the impossible bound !>=256 to (uint8_t)x. That bound reaches y through unsigned y = (uint8_t)x;. The forward analyzer (evalAssignment in vf_analyzers.cpp) then shifts the bound for -=/-- without the no-wrap check that the PR added for binary +/-/* in setTokenValue. So y -= 1; if (y > 255) is reported as always false, but it is true for x == 0. The plain form y = y - 1 is handled correctly.

The same analyzer bug already exists in base for bounds that come from conditions (if (y > 255) return; y -= 1; if (y > 255) is wrong in base too). This is the same situation as the earlier ~ finding: with the PR, every integral cast reaches the bug.

The tests I ran pass. Apart from this, the cast and arithmetic cases I tried gave no new false positives. The problems I saw that are already in comments.md (plain char, unsigned loop index, && loops) I did not report again.

Testing

Build: I built cppcheck and testrunner in pr/, and cppcheck in base/. Both built without errors.

Tests:

• These suites all give Tests failed: 0: TestCondition, TestValueFlow, TestBufferOverrun, TestStl, TestType, TestOther, TestFunctions, TestUninitVar and TestSymbolDatabase.
• In test/cli/other_test.py, -k "15000 or missing_include or cast or floating or bounded or 6366" gives 7 passed.

New false positive: compound decrement after a cast

c.c:

#include <stdint.h>
void use(int);
void g4(int x) { uint32_t y = (uint8_t)x; y -= 1; if (y > 255) use(1); }
void g5(int x) { unsigned y = (uint8_t)x; y -= 1; if (y > 255) use(2); }
void g6(int x) { unsigned y = (uint8_t)x; y = y - 1; if (y > 255) use(3); }
void g7(int x) { unsigned y = (uint8_t)x; y--; if (y > 255) use(4); }
void g8(int x) { unsigned y = (uint8_t)x; if (y - 1 > 255) use(5); }
void g9(unsigned char c) { unsigned y = c; y -= 1; if (y > 255) use(6); }

Command: cppcheck -q --enable=style,warning --platform=unix64 --template='{line}:{id}:{message}' c.c

• Base: no output.
• PR:

3:knownConditionTrueFalse:Condition 'y>255' is always false
4:knownConditionTrueFalse:Condition 'y>255' is always false
6:knownConditionTrueFalse:Condition 'y>255' is always false

All three conditions are true for x == 0. y = y - 1 (line 5) and y - 1 > 255 (line 7) are handled correctly.

--debug on the g5 line shows that the cast bound is shifted with no wrap check:

( always {!<=-1,!>=256}
y {symbolic=((uint8_t)x-1),!<=-1,!>=255}

A realistic case: size_t r = (uint16_t)n; r -= 1; if (r >= 65535) return 0;. The PR reports r>=65535 as always false; base does not.

The same analyzer bug is already in base for bounds from conditions, so this is not new code: void k1(unsigned y) { if (y > 255) return; y -= 1; if (y > 255) use(1); } is reported as always false by both base and PR.

Other cases that matched base or improved on it:

• Arithmetic on casts, on unix64, win64 and unix32: *// by negative constants, 300 - (uint8_t)x, >>, <<, |, &, %, (uint8_t)x ± (uint8_t)y, and -(int8_t)x.
• Other targets: enum casts with an underlying type, size_t, uintptr_t and unsigned long casts, (unsigned)(p - q), and assignments to int followed by +=/-=.
• The new results I got were correct, for example 300-(uint8_t)x<45 and int y=(uint8_t)x; y-=256; y>=0 are always false. I found no other false positives.

Automated review of 04fb4da by Claude. The pull request was built and tested locally.

AI review

One new false positive: when a value comes from a cast to a narrow signed type and is then incremented or decremented in place (i++, i += 1, i--), it is reported as never wrapping. The PR puts the bounds !<=-129,!>=128 on (signed char)x. These bounds reach i through signed char i = (signed char)x;. After i++, the bound is shifted to !<=-127. That happens because the new guard at lib/vf_analyzers.cpp:286 only stops this for UNSIGNED types. But char and short are promoted to int and then converted back, so 127 + 1 becomes -128 in practice (implementation-defined in C, defined modulo 2^N since C++20). The result is new "always false" warnings for common wrap checks like if (i == -128) and if (s == -32768). Base gives none of them.

The unsigned version of this problem (y -= 1 and y--) was raised before and is now fixed. This signed narrow case is new. Apart from it I found no new problems. The other results I saw match base or improve on it, or are already in comments.md.

Testing

Build: I built cppcheck and testrunner in pr/, and cppcheck in base/. Both built without errors.

Unit tests: These suites all give Tests failed: 0: TestCondition, TestValueFlow, TestBufferOverrun, TestType, TestStl, TestOther, TestFunctions, TestUninitVar and TestSymbolDatabase.

New false positive: in-place inc/dec on a narrow signed type after a cast

d.c:

void use(int);
void d1(int x) { signed char i = (signed char)x; i++; if (i == -128) use(1); }
void d2(int x) { short i = (short)x; i++; if (i == -32768) use(2); }
void d3(int x) { short i = (short)x; i += 1; if (i == -32768) use(3); }
void d4(int x) { short i = (short)x; i--; if (i == 32767) use(4); }
void d5(signed char i) { i++; if (i == -128) use(5); }
void d6(int x) { signed char i = (signed char)x; i = i + 1; if (i == -128) use(6); }

Command: cppcheck -q --enable=style,warning --platform=unix64 --template='{line}:{id}:{message}' d.c

Base: no output for these lines. PR:

2:knownConditionTrueFalse:Condition 'i==-128' is always false
3:knownConditionTrueFalse:Condition 'i==-32768' is always false
4:knownConditionTrueFalse:Condition 'i==-32768' is always false
5:knownConditionTrueFalse:Condition 'i==32767' is always false

Each condition is true for some x, for example x == 127, x == 32767 or x == -32768. i = i + 1 (d6) is handled correctly. So is a plain parameter (d5).

--debug for d1 shows the bound being shifted with no wrap check:

( always {!<=-129,!>=128}
++ {!<=-129,!>=128,symbolic=((char)x),127}
i {symbolic=((char)x+1),!<=-127}

The same happens with int8_t (int8_t i = (int8_t)x; i++; if (i == -128)).

Other checks (same as base, or better):

• I tried about 25 cast and arithmetic cases, all with no false positives. They covered multiplying or dividing by negative constants, 0 - (int8_t)x, ^, |, %, >>, <<, !, mixed signed/unsigned cast operands, and *= / -= / unary - on int variables that hold a cast value.
• I tried about 20 C++ cases: static_cast to an enum with an underlying type, enum class, bool, char16_t, wchar_t, functional casts and chained casts. They ran on unix64 and win64 with no new false positives.
• The PR removes base false positives such as uint8_t i = (uint8_t)x; i++; if (i == 0), uint16_t i += 1, unsigned char c--; c == 255, int r = (uint8_t)x; r *= -1; r == -255, and (bool)x + 1 == 2.
• Larger files: tinyxml2.cpp, simplecpp.cpp and test/cfg/std.c took the same time with base and the PR (21/22 s, 526/530 s, 5/5 s with the debug build). The warnings were identical. The only difference was one extra "Assuming that condition … is not redundant" note in tinyxml2, a change in error paths that comments.md already mentions.

Automated review of f706471 by Claude. The pull request was built and tested locally.